Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ENTSO-E parser to use new proxy #6312

Merged
merged 4 commits into from
Jan 9, 2024
Merged

Conversation

madsnedergaard
Copy link
Member

Description

Changes ENTSO-E parser to query via a new proxy service.

Double check

  • I have tested my parser changes locally with poetry run test_parser "zone_key"

@github-actions github-actions bot added parser python Pull requests that update Python code labels Jan 5, 2024
Copy link
Contributor

github-actions bot commented Jan 5, 2024

PR Analysis

  • 🎯 Main theme: Updating ENTSO-E parser to use a new proxy service
  • 📝 PR summary: This PR updates the ENTSO-E parser to use a new proxy service. It simplifies the code by removing the need to shuffle and try multiple tokens, and instead uses a single token for the new proxy service. It also updates the script ENTSOE_capacity_update.py to reflect this change.
  • 📌 Type of PR: Enhancement
  • 🧪 Relevant tests added: No
  • ⏱️ Estimated effort to review [1-5]: 2, because the changes are straightforward and mainly involve updating URLs and removing redundant code.
  • 🔒 Security concerns: No

PR Feedback

💡 General suggestions: The changes in this PR are clear and straightforward. However, it would be beneficial to add tests to ensure that the new proxy service works as expected. This could include tests to check the response from the new proxy service and handle any potential errors.

🤖 Code feedback:
relevant fileparsers/ENTSOE.py
suggestion      

Consider handling the case where the get_token function returns None. Currently, if get_token returns None, it will be passed as a parameter to the session.get call, which may lead to unexpected behavior. [important]

relevant linetoken = get_token("ENTSOE_TOKEN")

relevant filescripts/ENTSOE_capacity_update.py
suggestion      

Similar to the previous suggestion, consider handling the case where get_token returns None in this script as well. [important]

relevant linetoken = args.api_token or get_token("ENTSOE_TOKEN")

✨ Usage tips:

To invoke the PR-Agent, add a comment using one of the following commands:

  • /review: Request a review of your Pull Request.
  • /describe: Update the PR title and description based on the contents of the PR.
  • /improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
  • /ask <QUESTION>: Ask a question about the PR.
  • /update_changelog: Update the changelog based on the PR's contents.
  • /add_docs 💎: Generate docstring for new components introduced in the PR.
  • /generate_labels 💎: Generate labels for the PR based on the PR's contents.
  • /analyze 💎: Automatically analyzes the PR, and presents changes walkthrough for each component.

See the tools guide for more details.
To edit any configuration parameter from the configuration.toml, add --config_path=new_value.
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, add a /config comment.

parsers/ENTSOE.py Show resolved Hide resolved
parsers/ENTSOE.py Show resolved Hide resolved
scripts/ENTSOE_capacity_update.py Show resolved Hide resolved
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's working perfectly!

There was a reason the checklist had two boxes though 😉

Copy link
Member

@VIKTORVAV99 VIKTORVAV99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's working perfectly!

There was a reason the checklist had two boxes though 😉

@VIKTORVAV99
Copy link
Member

Looks like we are mocking the tokens in a test too, we need to update that.

@madsnedergaard
Copy link
Member Author

Looks like it's working perfectly!

There was a reason the checklist had two boxes though 😉

Haha, I've always just read the "pnpm ..." and assumed it was only about frontend 😅
Also thought we had pre-commit running for contrib, but seems like we never add ruff to the checks? What's missing from replacing isort and black completely there?

@github-actions github-actions bot added the tests label Jan 5, 2024
@VIKTORVAV99
Copy link
Member

VIKTORVAV99 commented Jan 5, 2024

Looks like it's working perfectly!
There was a reason the checklist had two boxes though 😉

Haha, I've always just read the "pnpm ..." and assumed it was only about frontend 😅 Also thought we had pre-commit running for contrib, but seems like we never add ruff to the checks? What's missing from replacing isort and black completely there?

Pretty sure I uninstalled isort and black, I don't think we ever had all of them in there since they where so slow...

@madsnedergaard
Copy link
Member Author

Looks like it's working perfectly!
There was a reason the checklist had two boxes though 😉

Haha, I've always just read the "pnpm ..." and assumed it was only about frontend 😅 Also thought we had pre-commit running for contrib, but seems like we never add ruff to the checks? What's missing from replacing isort and black completely there?

Pretty sure I uninstalled isort and black, I don't think we ever had all of them in there since they where so slow...

On the monorepo we still run isort and black on pre-commit, so maybe it's time to replace that :)

@VIKTORVAV99
Copy link
Member

VIKTORVAV99 commented Jan 5, 2024

Looks like it's working perfectly!
There was a reason the checklist had two boxes though 😉

Haha, I've always just read the "pnpm ..." and assumed it was only about frontend 😅 Also thought we had pre-commit running for contrib, but seems like we never add ruff to the checks? What's missing from replacing isort and black completely there?

Pretty sure I uninstalled isort and black, I don't think we ever had all of them in there since they where so slow...

On the monorepo we still run isort and black on pre-commit, so maybe it's time to replace that :)

Yeah, I noticed we where still using black, led to some frustration when there was minor differences between ruff and black and I had autosave and format on save on and ruff as the default 😅.

But I can take care of the pre-commit hook in here, then we can start replacing things on the backend too.

@madsnedergaard
Copy link
Member Author

Will merge and release this to production tomorrow so I can watch in case issues occur! :)

@madsnedergaard madsnedergaard enabled auto-merge (squash) January 9, 2024 09:01
@madsnedergaard madsnedergaard enabled auto-merge (squash) January 9, 2024 12:50
@madsnedergaard madsnedergaard merged commit 7cde8b4 into master Jan 9, 2024
12 checks passed
@madsnedergaard madsnedergaard deleted the mn/entsoe-proxy branch January 9, 2024 12:51
ghost pushed a commit that referenced this pull request Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parser python Pull requests that update Python code tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants